Add sparse stack#2014
Conversation
|
I see the spell check failed, will work on it. |
yhmtsai
left a comment
yhmtsai
left a comment
There was a problem hiding this comment.
mostly format, but try to go through the code
| static std::unique_ptr<SparseStack> create( | ||
| std::shared_ptr<const Executor> exec, size_type sketch_size, | ||
| size_type input_size, size_type zeta, uint64 seed); |
There was a problem hiding this comment.
I thought you will also need create with executor for default contruction
There was a problem hiding this comment.
I mean static std::unique_ptr<SparseStack> create(std::shared_ptr<const Executor> exec)
| * Each input row i is mapped to zeta output rows hash_map[i * zeta + z] | ||
| * with signs signs[i * zeta + z] for z in [0, zeta). |
There was a problem hiding this comment.
Is zeta in the "zeta output rows" the same meaning as i * zeta + z?
the operation is not complex, but does any initial paper provide it?
pratikvn
left a comment
pratikvn
left a comment
There was a problem hiding this comment.
Looks good! I think we can probably merge this as soon as you add some tests (See core/test/*_sketch_kernels.cpp).
We can focus on getting the functionality in and benchmarking it, as we are not really merging it into develop yet.
| * | ||
| * Each input row i is mapped to zeta output rows hash_map[i * zeta + z] | ||
| * with signs signs[i * zeta + z] for z in [0, zeta). | ||
| * Only O(zeta * m) storage is needed (no explicit matrix). |
There was a problem hiding this comment.
Maybe also mention how it is different (possible advantages) to CountSketch ?
Co-authored-by: Yu-Hsiang M. Tsai <19565938+yhmtsai@users.noreply.github.com>
Co-authored-by: Yu-Hsiang M. Tsai <19565938+yhmtsai@users.noreply.github.com>
Co-authored-by: Yu-Hsiang M. Tsai <19565938+yhmtsai@users.noreply.github.com>
Co-authored-by: Yu-Hsiang M. Tsai <19565938+yhmtsai@users.noreply.github.com>
Co-authored-by: Yu-Hsiang M. Tsai <19565938+yhmtsai@users.noreply.github.com>
Co-authored-by: Yu-Hsiang M. Tsai <19565938+yhmtsai@users.noreply.github.com>
|
The most important issues should be resolved now, maybe we can merge and focus on the remaining issues later? |
| // Added zeta = 1 | ||
| auto sketch = TestFixture::Sketch::create(this->exec, 3, 5, 1, 42); | ||
| auto b = Dense::create(this->exec, gko::dim<2>{4, 5}); | ||
| // x must have rows == 3 (sketch_size), this has 4 |
There was a problem hiding this comment.
I think here, // x must have rows == b->rows (4) ? This test seems to pass now, as the dimension of x is actually correct.
|
I think there is an issue with the test: https://gitlab.com/ginkgo-project/ginkgo-public-ci/-/jobs/14328574363#L4481. See review comment |
3 participants
Add SparseStack class, using the same logic as CountSketch and gaussian.
No concerns for optimization yet.